Skip to content

Feat/add metadata#1531

Closed
walldenfilippa wants to merge 7 commits intoAltinn:mainfrom
walldenfilippa:Feat/AddMetadata
Closed

Feat/add metadata#1531
walldenfilippa wants to merge 7 commits intoAltinn:mainfrom
walldenfilippa:Feat/AddMetadata

Conversation

@walldenfilippa
Copy link

@walldenfilippa walldenfilippa commented Oct 21, 2025

Description

Added metadata to the data element, along with functionality to store the metadata within it.
This is part of the new feature thumbnail in the fileupload component, related PR in frontend (Altinn/app-frontend-react#3797) and localtest (Altinn/app-localtest#183). Storage (in process).

Related Issue(s)

  • #{issue number}

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)
  • I'll complete the documentation once the work is finished.

Summary by CodeRabbit

  • New Features
    • Support for attaching metadata to newly created data elements; metadata is persisted immediately and the element is then locked to prevent further modification.
  • Chores
    • Added a small helper method to enable attaching metadata where supported.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

Warning

Rate limit exceeded

@walldenfilippa has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 46 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between c738709 and e46b828.

📒 Files selected for processing (1)
  • src/Altinn.App.Core/Models/DataElementChanges.cs (2 hunks)
📝 Walkthrough

Walkthrough

Adds metadata storage and runtime API to attach metadata to created data elements, persists that metadata during binary data creation, and marks changes as locked after persistence.

Changes

Cohort / File(s) Summary
Data element metadata model
src/Altinn.App.Core/Models/DataElementChanges.cs
Adds a private _metadata field, an internal IReadOnlyCollection<KeyValueEntry>? Metadata property, an internal bool Lock flag, and a public AddMetadata(string key, string value) method with validation and guards to attach metadata to created data elements.
Persist metadata on creation
src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
After InsertBinaryData, if change.Metadata is present, assigns it to the created DataElement, calls _dataClient.Update to persist the metadata, and sets change.Lock = true.
Public API verification
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt
Public API baseline updated to include the new void AddMetadata(string key, string value) method on DataElementChange.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review validation in AddMetadata (null/whitespace checks, ChangeType == Created, Lock guard).
  • Confirm _dataClient.Update call correctly handles persistence and error paths in InstanceDataUnitOfWork.
  • Ensure Lock semantics do not break other flows that may modify DataElementChange after creation.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Feat/add metadata' directly matches the main change: adding metadata functionality to data elements. It clearly summarizes the primary purpose of the changeset.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac8ddf4 and 6382df0.

📒 Files selected for processing (3)
  • src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1 hunks)
  • src/Altinn.App.Core/Models/DataElementChanges.cs (2 hunks)
  • test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.cs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.cs: Use internal accessibility on types by default
Use sealed for classes unless inheritance is a valid use-case
Dispose IDisposable/IAsyncDisposable instances
Do not use .GetAwaiter().GetResult(), .Result(), .Wait(), or other blocking APIs on Task
Do not use the Async suffix for async methods
Write efficient code; avoid unnecessary allocations (e.g., avoid repeated ToString calls; consider for-loops over LINQ when appropriate)
Do not invoke the same async operation multiple times in the same code path unless necessary
Avoid awaiting async operations inside tight loops; prefer batching with a sensible upper bound on parallelism
Use CSharpier for formatting (required before commits; formatting also runs on build via CSharpier.MSBuild)

Files:

  • src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
  • src/Altinn.App.Core/Models/DataElementChanges.cs
**/*.{cs,csproj}

📄 CodeRabbit inference engine (CLAUDE.md)

Use Nullable Reference Types

Files:

  • src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
  • src/Altinn.App.Core/Models/DataElementChanges.cs
src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs

📄 CodeRabbit inference engine (CLAUDE.md)

src/{Altinn.App.Api,Altinn.App.Core}/**/*.cs: Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
For HTTP APIs, define DTOs with ...Request and ...Response naming

Files:

  • src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
  • src/Altinn.App.Core/Models/DataElementChanges.cs
🧠 Learnings (1)
📚 Learning: 2025-09-18T07:40:01.660Z
Learnt from: ivarne
PR: Altinn/app-lib-dotnet#1470
File: src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs:411-418
Timestamp: 2025-09-18T07:40:01.660Z
Learning: In InstanceDataUnitOfWork.UpdateDataElement, the _binaryCache is intentionally NOT updated after successful UpdateBinaryData calls. This preserves the ability to repeatedly detect which elements were changed during a session, even after they've been saved to storage. The "dirty" detection is by design for audit/tracking purposes.

Applied to files:

  • src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Build and Test
  • GitHub Check: Run dotnet build and test (windows-latest)
  • GitHub Check: Run dotnet build and test (macos-latest)
  • GitHub Check: Run dotnet build and test (ubuntu-latest)
  • GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
src/Altinn.App.Core/Models/DataElementChanges.cs (2)

76-79: LGTM!

The internal Metadata property correctly exposes the metadata collection as read-only.


81-84: LGTM!

The Lock flag is appropriately scoped as internal and provides a simple mechanism to prevent further metadata changes.

test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)

3964-3964: Public API baseline update acknowledged.

The new AddMetadata(string key, string value) signature matches the intended metadata workflow for created data elements. No further action needed.

Comment on lines 45 to 48
/// <summary>
/// The metadata of the data element
/// </summary>
private List<KeyValueEntry>? _metadata = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Contradictory initialization logic.

The field is declared nullable and initialized to an empty list, but line 100 uses lazy initialization (_metadata ??= []). Either remove the initializer here (for lazy init) or remove the nullable annotation and line 100's null-coalescing (for eager init).

Apply this diff for lazy initialization (recommended):

-    private List<KeyValueEntry>? _metadata = [];
+    private List<KeyValueEntry>? _metadata;

Or apply this diff for eager initialization:

-    private List<KeyValueEntry>? _metadata = [];
+    private List<KeyValueEntry> _metadata = [];

And remove line 100's null-coalescing:

-        _metadata ??= [];
-        _metadata.Add(new KeyValueEntry { Key = key, Value = value });
+        _metadata.Add(new KeyValueEntry { Key = key, Value = value });

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/Altinn.App.Core/Models/DataElementChanges.cs around lines 45-48 (and note
usage at line 100), the private field _metadata is declared nullable but eagerly
initialized to an empty list which conflicts with the lazy-init pattern used at
line 100; remove the initializer (leave "private List<KeyValueEntry>?
_metadata;") so the field remains nullable and the existing "_metadata ??= []"
lazy initialization at line 100 works as intended.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/Altinn.App.Core/Models/DataElementChanges.cs (1)

103-104: Critical bug: Metadata list is overwritten on each call.

Line 103 unconditionally assigns a new empty list to _metadata, which discards all previously added metadata entries. Only the last entry added will be retained.

Apply this diff to use lazy initialization:

-        _metadata =  [];
+        _metadata ??= [];
         _metadata.Add(new KeyValueEntry { Key = key, Value = value });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6382df0 and e46fc82.

📒 Files selected for processing (2)
  • src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1 hunks)
  • src/Altinn.App.Core/Models/DataElementChanges.cs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns

Files:

  • src/Altinn.App.Core/Models/DataElementChanges.cs
🧠 Learnings (1)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
🪛 GitHub Actions: Verify dotnet format
src/Altinn.App.Core/Models/DataElementChanges.cs

[error] 108-108: CSharpier formatting check failed. File is not formatted as expected around line 108. Run 'dotnet csharpier check .' or 'dotnet csformatter' to fix formatting.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Analyze (csharp)
  • GitHub Check: Run dotnet build and test (macos-latest)
  • GitHub Check: Run dotnet build and test (ubuntu-latest)
  • GitHub Check: Run dotnet build and test (windows-latest)
  • GitHub Check: Build and Test
🔇 Additional comments (4)
src/Altinn.App.Core/Models/DataElementChanges.cs (4)

45-48: Field declaration corrected.

The contradictory initialization has been resolved. The field is now properly declared without an initializer, supporting the lazy initialization pattern.


76-79: LGTM!

The property correctly exposes the metadata as a read-only collection while maintaining proper encapsulation.


81-84: LGTM!

The Lock property provides a simple and effective mechanism to prevent metadata modifications after persistence.


91-92: Parameter validation implemented correctly.

The validation ensures that both key and value are not null, empty, or whitespace, addressing the previous review comment.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e46fc82 and d2f9f10.

📒 Files selected for processing (2)
  • src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs (1 hunks)
  • src/Altinn.App.Core/Models/DataElementChanges.cs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Altinn.App.Core/Internal/Data/InstanceDataUnitOfWork.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns

Files:

  • src/Altinn.App.Core/Models/DataElementChanges.cs
🧠 Learnings (3)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.

Applied to files:

  • src/Altinn.App.Core/Models/DataElementChanges.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild

Applied to files:

  • src/Altinn.App.Core/Models/DataElementChanges.cs
🪛 GitHub Actions: Verify dotnet format
src/Altinn.App.Core/Models/DataElementChanges.cs

[error] 108-108: dotnet csharpier formatting check failed. Expected: Around Line 108: _metadata = []; Actual: Around Line 108: _metadata = []; There is extra spacing around the assignment. Run 'dotnet csharpier check .' or apply formatting to fix.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Build and Test
  • GitHub Check: Run dotnet build and test (macos-latest)
  • GitHub Check: Analyze (csharp)
  • GitHub Check: Run dotnet build and test (ubuntu-latest)
  • GitHub Check: Run dotnet build and test (windows-latest)

@walldenfilippa
Copy link
Author

/publish

@github-actions
Copy link

github-actions bot commented Dec 11, 2025

PR release:

⚙️ Building...
✅ Done!

@walldenfilippa walldenfilippa deleted the Feat/AddMetadata branch December 12, 2025 08:51
@walldenfilippa walldenfilippa added backport-ignore This PR is a new feature and should not be cherry-picked onto release branches feature Label Pull requests with new features. Used when generation releasenotes labels Dec 12, 2025
@walldenfilippa
Copy link
Author

new PR: #1598

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-ignore This PR is a new feature and should not be cherry-picked onto release branches feature Label Pull requests with new features. Used when generation releasenotes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant